Skip to content

Remove IPA / Part1#377

Merged
adria0 merged 5 commits into
mainfrom
feat/remove_ipa
Nov 11, 2024
Merged

Remove IPA / Part1#377
adria0 merged 5 commits into
mainfrom
feat/remove_ipa

Conversation

@adria0

@adria0 adria0 commented Oct 18, 2024

Copy link
Copy Markdown

In this first part, IPA functionality is removed.
Some traits still exists ( like CommitmentScheme, but I'll do it in the second part )

@adria0 adria0 force-pushed the feat/remove_ipa branch 3 times, most recently from aa26952 to 747d837 Compare October 18, 2024 11:14
@codecov-commenter

codecov-commenter commented Oct 18, 2024

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 83.87%. Comparing base (433dfbb) to head (b6f4082).

Files with missing lines Patch % Lines
halo2_backend/src/poly/query.rs 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #377      +/-   ##
==========================================
- Coverage   85.14%   83.87%   -1.28%     
==========================================
  Files          85       76       -9     
  Lines       18872    17583    -1289     
==========================================
- Hits        16068    14747    -1321     
- Misses       2804     2836      +32     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@adria0 adria0 changed the title Remove IPA Remove IPA / Part1 Oct 22, 2024
@adria0 adria0 marked this pull request as ready for review October 22, 2024 08:21

@guorong009 guorong009 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that blind feature is removed through whole codebase.
I think blind is needed for privacy feature of ZK.
Why do we remove that feature?

Comment thread book/src/design/proving-system/inner-product.md
Comment thread book/src/user/experimental-features.md
Comment thread halo2_backend/src/helpers.rs
Comment thread halo2_proofs/tests/plonk_api.rs
Comment thread halo2_backend/src/poly/commitment.rs
Comment thread book/src/design/proving-system/inner-product.md
Comment thread book/src/user/experimental-features.md
Comment thread halo2_backend/src/poly/commitment.rs
compressed_input_expression: Polynomial<C::Scalar, LagrangeCoeff>,
permuted_input_expression: Polynomial<C::Scalar, LagrangeCoeff>,
permuted_input_poly: Polynomial<C::Scalar, Coeff>,
permuted_input_blind: Blind<C::Scalar>,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are the blinders removed? 🤔

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto (Please, take note that I removed everything that the compiler said as "unused", that are just few things.)

@guorong009 guorong009 Oct 25, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some research.
Found that blinders are really used in IPA commitment scheme.
But, when @kilic and @han0110 introduce the KZG commitment scheme, they didn't use this blinder/r in commitment scheme.
51d523d#diff-928bb816879ab4beb4d0ad6d02b06d158f760aa2e95f410b2694e0f803e57486R167-R171
I believe the reason is that it(ParamsKZG::commit_lagrange) should be compatible with existing IPA scheme, at that time.
In other words, the blinder/r is not needed for KZG scheme.

If so, we can safely remove all of these blinder/r features from codebase.
Plus, update ParamsKZG::commit_lagrange function definition.

Overall, I think it is better to get to know exact reason, before removing this feature from codebase.

@adria0 adria0 Oct 25, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I think it is better to get to know exact reason, before removing this feature from codebase.

Sure, more knowledge is always good.

@guorong009, are you talking about changing the function definition from

    /// This commits to a polynomial using its evaluations over the $2^k$ size
    /// evaluation domain. The commitment will be blinded by the blinding factor
    /// `r`.
    fn commit_lagrange(
        &self,
        engine: &impl MsmAccel<C>,
        poly: &Polynomial<C::ScalarExt, LagrangeCoeff>,
        r: Blind<C::ScalarExt>,
    ) -> C::CurveExt;

to ?

    /// This commits to a polynomial using its evaluations over the $2^k$ size
    /// evaluation domain.
    fn commit_lagrange(
        &self,
        engine: &impl MsmAccel<C>,
        poly: &Polynomial<C::ScalarExt, LagrangeCoeff>
    ) -> C::CurveExt;

if yes, this needs refactoring in other parts of the code that I think that is better to do it in another PR.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, Params::commit function. @adria0

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking a bit more into this. I hadn't realized that KZG was completely ignoring blinders.
I don't think there is any bugs in the proving system though, because it was always being called with Blind::default().

It seems wrong that there is no possibility to blind the witnesses... where is the zk then?
This has been a point of discussion in the past (#73 #76) but I ignore what came out of these.

@han0110 Could you comment on this? Are we blinding witnesses at some other level, or did we just remove zk?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still have zk, by blinding witnesses at other level.
The ProverMulti::commit_phase has the logic of adding random scalars into blinding_factors rows of witness/advice columns.
The columns to be blinded or not, are determined by unblinded_advice_columns.
I think nobody uses this feature in circuit development.

In above 2 PRs, they discuss whether/how to remove the blinding_factors row logic from the codebase.
As long as we "blind" some rows of witness columns, there would be no way for exposing the witness polys as is.
Hence, I think we can remove extra blinder/r feature from codebase.
I believe they come from the IPA scheme which needs explicit hiding value - r, for poly commitment.

Anyway, hope to get confirmation from @han0110 or @kilic .

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. If that is the case I'm fine with removing the blinders 👍

@adria0

adria0 commented Oct 24, 2024

Copy link
Copy Markdown
Author

Why do we remove that feature?

Because its no longer used anywhere after IPA removal.

@davidnevadoc davidnevadoc left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After revisiting the blinders issue I think that it is fine to get them removed, since the ZK need is achieved with a different mechanism. So LGTM! 🚀
Looking forward to part 2 and the removal of the rest of IPA related code!

@adria0 adria0 merged commit 0661f46 into main Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants